Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major re-org to crate / repository structure #215

Merged
merged 14 commits into from
Jan 6, 2025

Conversation

Carter12s
Copy link
Collaborator

@Carter12s Carter12s commented Dec 10, 2024

Description

A major re-organization of the components of roslibrust per: https://github.com/orgs/RosLibRust/discussions/214

Some facets of this:

  • The core roslibrust crate now includes no backends by default. The rosbridge backend is now hidden behind the rosbridge feature flag. This means you don't pay to compile any backend that you aren't using.
  • The rosbridge backend moved under the rosbridge namespace in the roslibrust crate so it is no longer treated as a special backend in the naming conventions.
  • RosApi trait and implementations moved out of roslibrust and into their own crate. Also now generically written so they can be used with any backend.

Fixes

Closes: number

Checklist

  • Update CHANGELOG.md

@Carter12s Carter12s changed the title [DRAFT] Major re-org to split out roslibrust_common and ros1 Major re-org to crate structure Dec 17, 2024
@Carter12s Carter12s changed the title Major re-org to crate structure Major re-org to crate / repository structure Dec 17, 2024
@Carter12s Carter12s requested review from ssnover and lucasw December 17, 2024 18:58
@Carter12s
Copy link
Collaborator Author

@lucasw / @ssnover this is ready for a review now.

It is a pretty big change to overall crate structure, but I think aligns things much more where I want development to head.

I think it sets things up well to eventually include a ros2 backend with either zenoh or dds, and I think it sets us up very well to start making the generic traits for TopicProvider and ServiceProvider the a first class API that both allows folks to swap backends around and to use MockRos for testing.

@ssnover
Copy link
Collaborator

ssnover commented Dec 17, 2024

Reading on mobile right now, but just going to ask clarifying questions and I'll review later:

  • Is the intended entry point crate roslibrust_common now?
  • Should it maybe be called roslibrust_core?

@Carter12s
Copy link
Collaborator Author

For users the intended entrypoint is still roslibrust

The "standard" usage would be to depend on roslibrust with at least one backend enabled via a feature flag.

Depending on roslibrust with no backends enabled can be done (see rosapi) to implement generic behaviors without a particular backend specified as well.

Not expecting anyone outside of roslibrust development to depend on roslibrust_common and I'm super happy to rename that to roslibrust_core or potentially roslibrust_traits <- that is most specifically what is actually inside it at the moment.

roslibrust/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ssnover ssnover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nitpicks and questions about tracking todos.

example_package/Cargo.toml Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/Cargo.toml Outdated Show resolved Hide resolved
roslibrust/src/tests/ros1_native_integration_tests.rs Outdated Show resolved Hide resolved
roslibrust_common/src/lib.rs Outdated Show resolved Hide resolved
@@ -197,7 +194,7 @@ impl NodeServerHandle {

let md5sum;
let md5sum_res =
roslibrust_codegen::message_definition_to_md5sum(topic_type, msg_definition);
roslibrust_common::md5sum::message_definition_to_md5sum(topic_type, msg_definition);
match md5sum_res {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an aside, but perhaps:

let md5sum = roslibrust_common::md5sum::message_definition_to_md5sum(
    topic_type,
    msg_definition
).map_err(|err| {
    log::error!("{err:?}");
    Err(NodeError::IoError(io::ErrorKind::ConnectionAborted.into()))
})?;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to change the name of message_definition_to_md5sum to from_message_definition since it's already namespaced under md5sum module.

@Carter12s Carter12s merged commit 94b0c81 into master Jan 6, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants